Retain Manifest Formatting on pack Command#65
Conversation
ChekTek
commented
Jan 9, 2026
ChekTek
commented
- preserve manifest line endings (CRLF | LF)
- preserve manifest indentation style (tabs | spaces)
There was a problem hiding this comment.
Pull request overview
This PR enhances the pack command to preserve the original formatting of manifest files when modifying them. Previously, the manifest was always written with tab indentation and LF line endings; now it retains the original file's formatting style.
Changes:
- Detect and preserve original line ending style (CRLF vs LF)
- Detect and preserve original indentation style (tabs vs spaces)
- Apply detected formatting when stringifying the modified manifest
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/commands/pack.ts
Outdated
|
|
||
| // Detect the original indentation style (tabs or spaces) | ||
| const indentMatch = original.match(/^[\t ]+/m); | ||
| const indent = indentMatch?.[0].startsWith("\t") ? "\t" : indentMatch?.[0] ?? "\t"; |
There was a problem hiding this comment.
The indentation detection logic is complex and unclear. If the match finds spaces, it uses all matched spaces as the indent, but if it finds tabs, it only uses a single tab. This inconsistency could lead to incorrect indentation if the original file uses multiple tabs. Consider explicitly handling the tab case to use the full matched string, or document why single tab is preferred.
| const indent = indentMatch?.[0].startsWith("\t") ? "\t" : indentMatch?.[0] ?? "\t"; | |
| const indent = indentMatch?.[0] ?? "\t"; |
There was a problem hiding this comment.
I'm not sure I've ever seen double-tab indentation, but the suggested change does make sense. 😅
src/commands/pack.ts
Outdated
|
|
||
| // Preserve original line endings | ||
| if (lineEnding === "\r\n") { | ||
| stringified = stringified.replaceAll("\n", "\r\n"); |
There was a problem hiding this comment.
Using replaceAll to convert LF to CRLF will cause issues if the stringified content already contains any CRLF sequences (which could happen if manifest values contain embedded newlines). This would result in CRCRLF sequences. Consider using replace with a regex that specifically targets LF not preceded by CR: stringified.replace(/(?<!\r)\n/g, '\r\n')
| stringified = stringified.replaceAll("\n", "\r\n"); | |
| stringified = stringified.replace(/(?<!\r)\n/g, "\r\n"); |
src/commands/pack.ts
Outdated
There was a problem hiding this comment.
@ChekTek, we should consider applying format retention here too, or at minimum removing the "".repeat(4).
|
Works perfectly |
3 participants
